Conversation
If gorelease in not in "Release" mode, npm packages are not pushed, so we can avoid multitudes of snapshot packages being pushed to public registry
The hook runs after. If goreaser is not in release mode, npm will build in dry-run mode, so the packages are not pushed to registry
PR #719 Multi-Agent Review: "Add npm publishing and update documentation"Repository: kosli-dev/cli | Author: jbrejner | Status: DRAFT
Branch: The Review PanelFour agents with distinct personas reviewed this PR independently, then their findings were synthesized. Here's who was on the panel:
Unanimous Verdict: DO NOT MERGE (yet)All four agents independently reached the same conclusion: the PR has a sound architectural foundation (the esbuild-style per-platform binary distribution pattern is well-chosen) but has several blocking issues that would result in a non-functional npm package if published today. This aligns with the PR's own DRAFT status and the author's listed TODOs. The Big Three: Issues Every Agent FlaggedThese three issues were independently identified by all four reviewers, making them the highest-confidence findings: 1. The Missing
|
|
Claude finished @jbrejner's task in 2m 54s —— View job PR Review: Add npm publishing and update documentation
Static AnalysisAll Go checks pass: Issues Addressed Since Multi-Agent ReviewGood progress — the three major issues flagged by all reviewers have been fixed:
Remaining IssuesMust-fix:
Should-confirm:
Nice-to-have:
What Looks Good
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| find npm -name package.json ! -path "npm/wrapper/*" | while read -r f; do | ||
| PKG_DIR="$(dirname "$f")" | ||
| PKG_NAME="$(basename "$PKG_DIR")" | ||
| echo "Packing ${PKG_NAME}..." | ||
| (cd "$PKG_DIR" && npm pack) | ||
| if [ "$DRY_RUN" = false ]; then | ||
| echo "Publishing ${PKG_NAME}..." | ||
| (cd "$PKG_DIR" && npm publish --tag "$NPM_TAG") | ||
| fi | ||
| done |
There was a problem hiding this comment.
Bug: Partial publish leaves broken release. If one platform package fails to publish, set -e will abort the loop — but packages already published remain on npm with no matching wrapper. Users on those platforms will get the binary but the wrapper (with the bin shim) won't exist at that version.
Consider either:
- Publishing all packages in a first pass (
npm packonly), then a second pass fornpm publish— so you catch pack failures early. - Or at minimum, adding cleanup/retry logic so a partial publish doesn't leave the registry in an inconsistent state.
Also: the find | while read pipeline means set -e won't catch failures inside the loop on some bash versions (the loop runs in a subshell). Consider using a for loop with a glob or xargs instead.
| fi | ||
|
|
||
| # Inject version into all platform package.json files | ||
| find npm -name package.json -exec sed -i "s/\"version\": \"[^\"]*\"/\"version\": \"${VERSION}\"/" {} \; |
There was a problem hiding this comment.
Portability issue with sed -i. On macOS, BSD sed requires sed -i '' (empty backup extension), while GNU sed on Linux uses sed -i without an argument. This will break local development on macOS.
Consider using a portable pattern:
| find npm -name package.json -exec sed -i "s/\"version\": \"[^\"]*\"/\"version\": \"${VERSION}\"/" {} \; | |
| find npm -name package.json -exec sed -i.bak "s/\"version\": \"[^\"]*\"/\"version\": \"${VERSION}\"/" {} \; -exec rm {}.bak \; |
Or better yet, use node -e or jq to manipulate JSON properly instead of regex on JSON files.
| contents: | ||
| # The src and dst attributes also supports name templates | ||
| - src: dist/{{ .ProjectName }}_{{ .Os }}_{{ if .Amd64 }}{{ .Arch }}_v1{{ else if .Arm }}{{ .Arch }}_6{{ else if eq .Arch "arm64" }}{{ .Arch }}_v8.0{{ else }}{{ .Arch }}{{ end }}/kosli | ||
| - src: dist/{{ .ProjectName }}_{{ .Os }}_{{ if .Amd64 }}{{ .Arch }}_v1{{ else if .Arm }}{{ .Arch }}_6{{ else }}{{ .Arch }}{{ end }}/kosli |
There was a problem hiding this comment.
Intentional change? This removes the {{ else if eq .Arch "arm64" }}{{ .Arch }}_v8.0 clause from the nfpms contents.src template. This changes the expected path for arm64 Linux .deb/.rpm packages from dist/kosli_linux_arm64_v8.0/kosli to dist/kosli_linux_arm64/kosli.
If GoReleaser still produces the _v8.0 suffixed directory, this will break Linux ARM64 .deb/.rpm packaging. Please confirm this matches the directory names GoReleaser actually creates with the version you're using.
| if (!SUPPORTED[platform] || !SUPPORTED[platform][arch]) { | ||
| process.stderr.write( | ||
| `[kosli] Note: ${platform}/${arch} is not a supported platform.\n` + | ||
| `[kosli] The kosli binary will not be available on this system.\n` | ||
| ); | ||
| process.exit(0); |
There was a problem hiding this comment.
Consider: should unsupported platforms really exit 0? This means npm install @kosli/cli will "succeed" on unsupported platforms with no indication of failure in the exit code. A CI pipeline installing kosli on an unsupported platform would silently succeed, then fail later when trying to run kosli.
The bin/kosli shim already exits non-zero for unsupported platforms, so the postinstall could too — users who intentionally want to skip would use --ignore-scripts.
| "registry": "https://registry.npmjs.org", | ||
| "access": "public" | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing engines field. Consider adding a minimum Node.js version constraint:
"engines": { "node": ">=16.0.0" }This helps users on old Node versions get a clear error rather than cryptic failures. Node 16+ is reasonable since require.resolve and spawnSync are well-established by then.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
| @@ -1,4 +1,4 @@ | |||
| --- | |||
| --- | |||
There was a problem hiding this comment.
Bug: Leading space in frontmatter delimiter. Line 1 is --- (with a leading space) instead of ---. This will break Hugo's frontmatter parsing — the page won't render correctly.
| --- | |
| --- |
| find npm -name package.json ! -path "npm/wrapper/*" | while read -r f; do | ||
| PKG_DIR="$(dirname "$f")" | ||
| PKG_NAME="$(basename "$PKG_DIR")" | ||
| echo "Packing ${PKG_NAME}..." | ||
| (cd "$PKG_DIR" && npm pack) | ||
| if [ "$DRY_RUN" = false ]; then | ||
| echo "Publishing ${PKG_NAME}..." | ||
| (cd "$PKG_DIR" && npm publish --tag "$NPM_TAG") | ||
| fi | ||
| done |
There was a problem hiding this comment.
Bug: set -e doesn't catch failures inside find | while read pipeline. The while loop runs in a subshell (due to the pipe), so if npm pack or npm publish fails, the script will continue and the exit code is lost. This means a partial publish can happen silently.
Replace with a for loop over a glob or use process substitution:
| find npm -name package.json ! -path "npm/wrapper/*" | while read -r f; do | |
| PKG_DIR="$(dirname "$f")" | |
| PKG_NAME="$(basename "$PKG_DIR")" | |
| echo "Packing ${PKG_NAME}..." | |
| (cd "$PKG_DIR" && npm pack) | |
| if [ "$DRY_RUN" = false ]; then | |
| echo "Publishing ${PKG_NAME}..." | |
| (cd "$PKG_DIR" && npm publish --tag "$NPM_TAG") | |
| fi | |
| done | |
| while IFS= read -r f; do | |
| PKG_DIR="$(dirname "$f")" | |
| PKG_NAME="$(basename "$PKG_DIR")" | |
| echo "Packing ${PKG_NAME}..." | |
| (cd "$PKG_DIR" && npm pack) | |
| if [ "$DRY_RUN" = false ]; then | |
| echo "Publishing ${PKG_NAME}..." | |
| (cd "$PKG_DIR" && npm publish --tag "$NPM_TAG") | |
| fi | |
| done < <(find npm -name package.json ! -path "npm/wrapper/*") |
Or simpler: use a for loop with globbing: for f in npm/cli-*/package.json; do ...
| contents: | ||
| # The src and dst attributes also supports name templates | ||
| - src: dist/{{ .ProjectName }}_{{ .Os }}_{{ if .Amd64 }}{{ .Arch }}_v1{{ else if .Arm }}{{ .Arch }}_6{{ else if eq .Arch "arm64" }}{{ .Arch }}_v8.0{{ else }}{{ .Arch }}{{ end }}/kosli | ||
| - src: dist/{{ .ProjectName }}_{{ .Os }}_{{ if .Amd64 }}{{ .Arch }}_v1{{ else if .Arm }}{{ .Arch }}_6{{ else }}{{ .Arch }}{{ end }}/kosli |
There was a problem hiding this comment.
Question: Is the arm64 _v8.0 suffix removal intentional? The previous template had {{ else if eq .Arch "arm64" }}{{ .Arch }}_v8.0 which matched GoReleaser's default directory naming for arm64 builds. Removing it changes the expected path from dist/kosli_linux_arm64_v8.0/kosli to dist/kosli_linux_arm64/kosli.
This was also flagged in commit ee7fb466 ("Update goreleaser config to fix arm64 package #711"). If your GoReleaser version no longer produces the _v8.0 suffix, this is fine — but please confirm this matches the actual directory names GoReleaser creates, otherwise Linux ARM64 .deb/.rpm packaging will break.
| if (!SUPPORTED[platform] || !SUPPORTED[platform][arch]) { | ||
| process.stderr.write( | ||
| `[kosli] Note: ${platform}/${arch} is not a supported platform.\n` + | ||
| `[kosli] The kosli binary will not be available on this system.\n` | ||
| ); | ||
| process.exit(0); |
There was a problem hiding this comment.
Consider exiting non-zero for unsupported platforms. Currently this exits 0, which means npm install @kosli/cli will "succeed" on unsupported platforms. A CI pipeline would silently succeed here and only fail later when trying to run kosli. The bin/kosli shim already exits non-zero for unsupported platforms — the postinstall could match that behavior. Users who intentionally want to skip can use --ignore-scripts.
2 participants
Distribute kosli cli as npm package.
Install globally with
npm install -g @kosli/cliThe above will install the wrapper package which detects the current platform and architecture and then pull the appriate package with the correct binary.
Draft because I still need TODO